Skip to content

trie: db insertions + leaf callback parallelism#20465

Closed
holiman wants to merge 6 commits into
ethereum:masterfrom
holiman:walker
Closed

trie: db insertions + leaf callback parallelism#20465
holiman wants to merge 6 commits into
ethereum:masterfrom
holiman:walker

Conversation

@holiman
Copy link
Copy Markdown
Contributor

@holiman holiman commented Dec 17, 2019

When we do Commit on the trie when parents-of-leafs are encountered, we do two things:

  1. call db.insert, which can be a fairly heavy operation, and
  2. call the onleaf function for the leafs. This can also be somewhat heavy.

This PR splits this up, so that we can have one dedicated goroutine which handles the insertion and callbacks, and the main trie-walker can concentrate on iterating the trie.

Since the order of events (inserts and callback invocation) is unchanged by this PR, I don't think this has any buggy side-effects.

Here are the benchmark results:

name                           old time/op    new time/op    delta
CommitAfterHash/no-onleaf-6      5.10µs ±14%    4.28µs ± 8%  -16.06%  (p=0.000 n=10+9)
CommitAfterHash/with-onleaf-6    6.77µs ±19%    5.44µs ±18%  -19.71%  (p=0.000 n=10+10)

name                           old alloc/op   new alloc/op   delta
CommitAfterHash/no-onleaf-6      1.79kB ± 3%    1.94kB ± 5%   +8.65%  (p=0.000 n=10+10)
CommitAfterHash/with-onleaf-6    2.18kB ± 2%    2.22kB ± 2%   +1.81%  (p=0.004 n=10+10)

name                           old allocs/op  new allocs/op  delta
CommitAfterHash/no-onleaf-6        11.0 ± 0%      13.0 ± 0%  +18.18%  (p=0.000 n=10+10)
CommitAfterHash/with-onleaf-6      18.3 ± 4%      20.0 ± 0%   +9.29%  (p=0.000 n=10+10)

As can be expected, if there's an onleaf function provided, the gains are higher 20%, compared to 16% if there is no onleaf.
The benchmarks are pretty synthetic, though, and I imagine that there is a lot more work done in the insert section on a live node.

This PR should work pretty nicely even pre-byzantium, I'd guess.

The actual implementation can surely be cleaned up a bit, particularly the hack in trie.go could be made a bit more elegant.

@holiman holiman changed the title Walker trie: db updates + leaf calback paralellism Dec 17, 2019
@holiman holiman requested a review from karalabe December 17, 2019 22:24
@holiman holiman changed the title trie: db updates + leaf calback paralellism trie: db updates + leaf callback parallelism Dec 17, 2019
@holiman holiman changed the title trie: db updates + leaf callback parallelism trie: db insertions + leaf callback parallelism Dec 17, 2019
@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Dec 18, 2019

I added a benchmark on fixed-size tries, to measure what the impact is on smaller tries.

BenchmarkCommitAfterHashFixedSize/10-6         	   16898	     71678 ns/op	   41125 B/op	     270 allocs/op
BenchmarkCommitAfterHashFixedSize/100-6        	    3934	    278342 ns/op	  189006 B/op	    1292 allocs/op
BenchmarkCommitAfterHashFixedSize/1000-6       	     457	   2592712 ns/op	 1917490 B/op	   13264 allocs/op
BenchmarkCommitAfterHashFixedSize/10000-6      	      27	  41369979 ns/op	20820080 B/op	  135751 allocs/op

Before:

BenchmarkCommitAfterHashFixedSize/10-6             20391             59862 ns/op           37498 B/op        240 allocs/op
BenchmarkCommitAfterHashFixedSize/100-6             4272            254688 ns/op          178676 B/op       1157 allocs/op
BenchmarkCommitAfterHashFixedSize/1000-6             403           2910285 ns/op         1828260 B/op      11901 allocs/op
BenchmarkCommitAfterHashFixedSize/10000-6             28          42699494 ns/op        19927566 B/op     121839 allocs/op

The channel-based Commit is slower for trie sizes of <1000 items, so it would be good to retain the existing non-channel-based Commit for smaller tries. Either by having a measure of the number of changes in the trie (adding a counter to Update) or simply use a different method when invoking account commit versus storage commit.

@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Dec 18, 2019

I've put this up on a full sync benchmark against master: https://geth-bench.ethdevops.io/d/Jpk-Be5Wk/dual-geth?orgId=1&var-exp=mon08&var-master=mon09&var-percentile=50&from=1576664098483&to=now

@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Dec 19, 2019

Full sync in progress, it's just passed byzantium at 4.37M.
Screenshot_2019-12-19 Dual Geth - Grafana

The exp machine is, I believe, a bit slower than master, and the exp is a bit behind in terms of blocks (seems to be about ten minutes). However, looking at the account commit chart, it seems that this PR is a couple of of milliseconds (or, maybe 20-25%) faster than master per block.

@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Dec 19, 2019

This PR hit byzantium about 5 minutes after master. After byzantium (4.37M), it started inching closer to master, and around block 4.822M it overtook it.
Screenshot_2019-12-19 Dual Geth - Grafana(1)

This chart shows the account commit:
Screenshot_2019-12-19 Dual Geth - Grafana(2)

During the high-intensity periods after byz, master peaks at 26ms , while this one peaks at 16ms.

@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Dec 21, 2019 •
edited
Loading

Here's the last two days:
Screenshot_2019-12-21 Dual Geth - Grafana
(OBS: notice that although they look identical, the y-axis are differently labeled -- the experimental is considerably faster on account commit)
From block ~4.56M to 6.02M. Experimental leads by about 10 minutes at the end.

@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Dec 23, 2019

After about 5 days of sync, the experimental is ahead with about 35 minutes. They're at block 7.6M, and I'm about to replace the images with another experiment.

@holiman holiman mentioned this pull request Dec 23, 2019
Merged
@holiman
Copy link
Copy Markdown
Contributor Author

holiman commented Jan 4, 2020

Closing in favour of #20481

@holiman holiman closed this Jan 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@karalabe karalabe Awaiting requested review from karalabe

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@holiman